feat: Api response extensions#754
Conversation
…re data retrieving Add onXXX method to apply code when success or error happens with strong typing (not null) Add mapXXX method to transform response content
There was a problem hiding this comment.
Pull request overview
Adds Kotlin extension helpers to streamline handling of ApiResponse success/error cases (callbacks + mapping), and extends the core ApiResponse model with an isError() convenience method.
Changes:
- Introduces
ApiResponseextensions:onSuccess,onError,on,mapSuccess,mapError,mapwith Sentry reporting on invalid states. - Adds
ApiResponseException/ApiErrorExceptionused for reporting inconsistent API payloads. - Adds
isError()toApiResponsemodel.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| Network/src/main/kotlin/com/infomaniak/core/network/utils/apiResponse/ApiResponseExt.kt | New ApiResponse extension API for success/error callbacks and mapping with Sentry capture. |
| Network/src/main/kotlin/com/infomaniak/core/network/utils/apiResponse/ApiResponseException.kt | New exception for “success but data is null” cases. |
| Network/src/main/kotlin/com/infomaniak/core/network/utils/apiResponse/ApiErrorException.kt | New exception for “error but error payload is null” cases. |
| Network/Models/src/main/java/com/infomaniak/core/network/models/ApiResponse.kt | Adds isError() helper to support new extensions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| inline fun <T> ApiResponse<T>.onSuccess(block: T.() -> Unit): ApiResponse<T> = apply { | ||
| if (isSuccess()) { | ||
| data?.run(block) ?: Sentry.captureException(ApiResponseException(this)) | ||
| } |
There was a problem hiding this comment.
onSuccess assumes a non-null data payload when result == SUCCESS, but the generic type parameter T is unconstrained (it can be nullable, e.g. ApiResponse<Foo?>). In that case a null success payload would be treated as an error and reported to Sentry. Consider constraining the extension to T : Any (and same for mapSuccess) or explicitly documenting/handling the nullable-success case without logging an exception.
| inline fun <T, R> ApiResponse<T>.map( | ||
| onSuccess: T.() -> R, | ||
| onError: ApiError.() -> R | ||
| ): R? = mapSuccess(onSuccess) ?: mapError(onError) |
There was a problem hiding this comment.
map() uses mapSuccess(...) ?: mapError(...), which makes null an ambiguous value: a successful mapping that legitimately returns null will still fall through to the error branch check. Using an explicit when on isSuccess()/isError() (or result) avoids the ambiguity and makes it clearer which branch is executed.
| ): R? = mapSuccess(onSuccess) ?: mapError(onError) | |
| ): R? = when { | |
| isSuccess() -> mapSuccess(onSuccess) | |
| isError() -> mapError(onError) | |
| else -> null | |
| } |
|
Failed to generate code suggestions for PR |
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨No code suggestions found for the PR. |
No description provided.